Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

functionality for compression quality #23

Merged
merged 1 commit into from
Mar 1, 2024
Merged

functionality for compression quality #23

merged 1 commit into from
Mar 1, 2024

Conversation

emrekuru97
Copy link
Contributor

Implemented paramtric compression quality only if strem from camera is raw, it can be done when the strem from camera is compressed too but i tried to avoid double compression.

@emrekuru97 emrekuru97 mentioned this pull request Feb 21, 2024
Closed
christianrauch
christianrauch requested changes Feb 21, 2024
View reviewed changes
Copy link
Owner

@christianrauch christianrauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comments regarding the parameter, format conversion and error handling. For reference, this is the implementation of toCompressedImageMsg that you want to replace: https://github.com/ros-perception/vision_opencv/blob/066793a23e5d06d76c78ca3d69824a501c3554fd/cv_bridge/src/cv_bridge.cpp#L512-L535

It may be easier to just copy it and add the compression parameter to cv::imencode. Or better, send a PR to the vision_opencv repo with that additional parameter :-)

src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
@christianrauch
Copy link
Owner

From your comments, I can now see that you are essentially reimplementing CompressedPublisher::publish instead of CvImage::toCompressedImageMsg. The implementation of CompressedPublisher::publish is much more complex than CvImage::toCompressedImageMsg.

Do you think re-implemening toCompressedImageMsg with the compression quality option would be sufficient for you?

@emrekuru97
Copy link
Contributor Author

emrekuru97 commented Feb 22, 2024 •
edited
Loading

Yes it would be sufficient, if it's ok i will copy the function and make the necessary changes and update the pull request. But we still need to call cv_bridge::toCvShare to get a CvImage. Since the function toCompressedImageMsg works on CvImage. But i'll left the format blank so cv_bridge::toCvShare wont do any unnecessary converting.

@emrekuru97
Copy link
Contributor Author

I tryed to fix all requested changes, if necessary i can go to a new branch with only one commit(if no more issues are present) or let this as it is.

christianrauch
christianrauch requested changes Feb 22, 2024
View reviewed changes
Copy link
Owner

@christianrauch christianrauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from my comments, you also have to apply the code style to let the CI pass. You can use the .clang-format with clang-format, or an IDE, to do this automatically.

src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
christianrauch
christianrauch requested changes Feb 23, 2024
View reviewed changes
Copy link
Owner

@christianrauch christianrauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am mostly fine with this now. The major thing is now to move this into a function and document its source and license.

src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
christianrauch
christianrauch requested changes Feb 24, 2024
View reviewed changes
Copy link
Owner

@christianrauch christianrauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The code looks much cleaner now. Missing is not only the source and license hint for the function that was copied (compressImageMsg) and a minor style change.

src/CameraNode.cpp Outdated Show resolved Hide resolved
christianrauch
christianrauch requested changes Feb 28, 2024
View reviewed changes
Copy link
Owner

@christianrauch christianrauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still a bit worried about mixing the code with the copied implementation of CvImage::toCompressedImageMsg. Could you move this function either into its own library (cleaner) or into a stand-alone function outside of the CameraNode class (easier)?

src/CameraNode.cpp Outdated Show resolved Hide resolved
src/CameraNode.cpp Outdated Show resolved Hide resolved
@emrekuru97
Copy link
Contributor Author

I'll also try to get a PR in original_repo with additional default parameter to the function. Or with a new signature. May be if they merge we can thane change this too.

christianrauch
christianrauch requested changes Feb 29, 2024
View reviewed changes
src/CameraNode.cpp Outdated Show resolved Hide resolved
@christianrauch
Copy link
Owner

I'll also try to get a PR in original_repo with additional default parameter to the function. Or with a new signature. May be if they merge we can thane change this too.

That would be awesome and remove the need to keep this function here.

christianrauch
christianrauch requested changes Feb 29, 2024
View reviewed changes
Copy link
Owner

@christianrauch christianrauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now. Final request: Can you squash all your commits into one and rebase/force-push to the PR? There are many back and forth changes that are better suited for a single commit.

@emrekuru97 emrekuru97 changed the title implement adjustable compression for raw image functionality for compression quality Feb 29, 2024
@emrekuru97
Copy link
Contributor Author

emrekuru97 commented Feb 29, 2024 •
edited
Loading

This looks good now. Final request: Can you squash all your commits into one and rebase/force-push to the PR? There are many back and forth changes that are better suited for a single commit.

Squashed the final code into the first commit, and changed the commit to "functionality for compression quality".

christianrauch
christianrauch requested changes Mar 1, 2024
View reviewed changes
src/CameraNode.cpp Outdated Show resolved Hide resolved
@christianrauch christianrauch merged commit 1863ff4 into christianrauch:main Mar 1, 2024
1 check passed
@emrekuru97
Copy link
Contributor Author

Thanks :). Yesterday i also tryed to get a PR here in the original cv_bridge repo to the humble branch, but i'm not sure if that is the correct way since there are many request that are awaiting. Anyway if they will merge i'll remove the function and pass directly the parameter to the new version. I'll add the tag closes now.

@christianrauch christianrauch mentioned this pull request May 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christianrauch christianrauch christianrauch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emrekuru97 @christianrauch